Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Apr 9, 2019

This reverts commit c1aa022, #618.

We've made a lot of progress separating out the per-platform components since that went in, so take another run at formally separating approval for per-platform and core changes.

CC @flaper87, @tomassedovic, @hardys: any concerns about this? If so, point me at the code you're concerned about not being able to bump independently, and as long as it's not vendor/, I'll see if I can make the core/platform separation cleaner there.

This reverts commit c1aa022, openshift#618.

We've made a lot of progress separating out the per-platform
components since that went in, so take another run at formally
separating approval for per-platform and core changes.
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 9, 2019
@wking
Copy link
Member Author

wking commented Apr 9, 2019

e2e-aws:

Apr 09 16:56:44.767 E clusteroperator/kube-controller-manager changed Failing to True: ... self-signed cert (/var/run/kubernetes/kube-controller-manager.crt, /var/run/kubernetes/kube-controller-manager.key)\nfailed to create listener: failed to listen on 0.0.0.0:10257: listen tcp 0.0.0.0:10257: bind: address already in use\n"
...

Flaky tests:

[Feature:Platform][Smoke] Managed cluster should start all core operators [Suite:openshift/conformance/parallel]

Failing tests:

[Feature:Platform] Managed cluster should have no crashlooping pods in core namespaces over two minutes [Suite:openshift/conformance/parallel]

That's being tracked in rhbz#1691055.

/retest

@tomassedovic
Copy link
Contributor

Thanks @wking! I'm in favour of this, but I had a look at the current status of the OWNERS files to have a better idea of where we stand.

There's one that's clearly missing in pkg/destroy/openstack/. I've created a PR for that here: #1575

Then there's a bunch of OpenStack-related code in these files:

  • pkg/asset/cluster/{metadata,tfwars}.go
  • pkg/asset/installconfig/*.go
  • pkg/asset/machines/*.go

All of these have an openstack subdirectory with the right OWNERS file, but there is some OpenStack-related code in the files above (mostly in switch statements).

And finally, these two places have some OpenStack code as well, but no openstack subdir:

  • pkg/asset/manifests/*.go
  • data/data/manifests/openshift/

In general, most of what we do happens in the openstack subdirectories we can merge independently, but there are OpenStack-specific cases throughout the repo. Assuming the wider installer team is okay with shouldering some of that review burden, I'm fine with not being to merge those directly.

@tomassedovic
Copy link
Contributor

Oh, one more thing: what about docs?

We could add OWNERS to docs/user/openstack, but then there's still dev-openstack-howto.md and I'm in favour of being more relaxed when it comes to docs (a mistake there will not break anything per se).

But if we wanted to gate them too, it wouldn't be the end of the world.

@wking
Copy link
Member Author

wking commented Apr 10, 2019

Assuming the wider installer team is okay with shouldering some of that review burden, I'm fine with not being to merge those directly.

Yeah. And I think we want to make those platform switches as simple as possible before punting over to platform-specific sub-packages. High churn in the switch itself means we're doing it wrong.

We could add OWNERS to docs/user/openstack, but then there's still dev-openstack-howto.md and I'm in favour of being more relaxed when it comes to docs (a mistake there will not break anything per se).

I'd rather push the libvirt and OpenStack howtos down into subdirs (e.g. docs/dev/openstack/README.md, or under user/?). Then approval for those can be scoped to the platform's approvers.

I'm agnostic about whether we also extend approval for the cross-platform docs to all installer approvers, or leave them restricted to core approvers. How many per-platform approvers do you expect will care about the cross-platform docs?

@tomassedovic
Copy link
Contributor

Yeah, scoping the dev docs sounds good to me.

How many per-platform approvers do you expect will care about the cross-platform docs?

I don't know. I'm kind of thinking it's more likely that we'd update a general troubleshooting step or something than a code change, but I don't expect a huge influx of docs updates either.

@wking
Copy link
Member Author

wking commented Apr 10, 2019

Yeah, scoping the dev docs sounds good to me.

Done with #1587.

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 7fa6d2f into openshift:master Apr 23, 2019
@wking wking deleted the tighten-platform-owner-scoping branch April 23, 2019 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants